fix: init should not require event ingestion url in sdk key #104
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aarsilv
approved these changes
Nov 27, 2024
felipecsl
reviewed
Nov 27, 2024
|
|
||
| // e.g. newDefaultEventDispatcher throws an error if it cannot parse a valid event ingestion URL from the SDK key | ||
| try { | ||
| instance.setEventDispatcher(newEventDispatcher(apiKey)); |
There was a problem hiding this comment.
fixed in Eppo-exp/js-sdk-common#143 to no longer throw
I'd vote removing this try/catch since it should never throw after that change
There was a problem hiding this comment.
Sounds good. I'll close this out
sameerank
deleted the
sameeran/init-should-not-require-event-ingestion-url-in-sdk-key
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
labels: mergeable
Fixes: #issue
Motivation and Context
I was running a script to try out the
initfunction and saw the following exception thrown when I invokedinitwith a pre-existing SDK key.I think the SDK should continue to support old keys, or at least handle the exception so it doesn't get thrown outside the SDK's scope
Description
Here, I propose wrapping the event dispatcher instantiation with a try-catch. Other approaches, like handling this in
js-client-sdk-common, may be valid tooHow has this been tested?